Codecov Report

❌ Patch coverage is 76.45740% with 210 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.37%. Comparing base (81d1e20) to head (892cec6).

@@            Coverage Diff             @@
##             main    #3763      +/-   ##
==========================================
+ Coverage   75.30%   75.37%   +0.06%     
==========================================
  Files         433      438       +5     
  Lines       38664    39463     +799     
==========================================
+ Hits        29116    29744     +628     
- Misses       7547     7647     +100     
- Partials     2001     2072      +71     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Claude finished @ongyimeng's task in 6m 35s —— View job

PR Review: Standalone consensus WAL

• Read core WAL files (codec, record, wal_writer, wal_store, wal_index, prune_watermark, replay)
• Review consensus/driver integration & db layer changes
• Analyze for correctness, concurrency, resource leaks, error handling
• Post inline comments + summary

Reviewed the full change set with a focus on consensus-recovery correctness, durability ordering, concurrency, and resource handling. This is a well-engineered PR — the durability invariants are sound and the failure-injection test coverage is unusually thorough. I have no blockers.

Blocker

None.

Important

None. A few things I verified that are correct and worth recording:

• WAL tail repair uses physical offsets consistently. appendSync stores the offset returned by Writer.WriteRecord and recoverLatestWALTail uses offset.Physical / PhysicalFile; the fake writer in tests mirrors record.Writer physical offsets, so truncation targets line up. (wal_writer.go:82, :230)
• appendSync won't deadlock on the sync WaitGroup. On a WriteRecord error it returns before waitGroup.Wait(), so a never-signalled Done can't hang the flush. (wal_writer.go:67-76)
• Prune durability ordering is crash-safe. Watermark is fsync'd before WAL files are deleted (wal_store.go:210-217), and a stale watermark is harmless because the surviving WAL prune records re-drive pruneLiveEntriesUpTo on replay (replay.go:113). Recovery at any crash point stays consistent.
• Persisted chan error (buffer 1) is leak-free. Each sync.go path sends exactly once, and the buffer means a producer never blocks even when OnCommit returns early on ctx.Done() without draining. (sync.go:324/349/359/372, commit_listener.go:71)
• putFixedUvarint32 round-trips correctly for the full uint32 range as a fixed 5-byte uvarint (verified the bit layout incl. the terminating buf[4] = value>>28). (codec.go:135)
• WAL store is fully mutex-guarded; LoadAllEntries snapshots under lock; replay (isReplaying) correctly suppresses re-writing WAL entries, avoiding duplication.

Nit

1. db/memory/db.go:113 — Path() panics on MkdirTemp failure and silently makes the "persistent" WAL ephemeral under an in-memory DB. Details in the inline comment. Fix this →
2. wal_index.go:51 — panic(err) in updateIndexesFromCommittedRecords. These records were just built via setEntry, so entry() can't realistically fail, but a panic on the post-commit consensus path is a sharp edge. Consider surfacing it as a returned error from flushLocked instead.
3. consensus.go:93-103 — WAL store leak on a future Init error. No path between NewTendermintWALStore and the return currently fails, but the store opens file handles / a Pebble WAL manager and is only closed via Driver.Run. If a fallible step is ever added below it, it'd leak; a deferred-close-on-error guard would harden this.
4. wal_store.go:24 — WAL retention. With cleanupPruneRecordInterval = 256 and ~one prune/height, up to ~256 heights of sealed WAL accumulate between cleanups. Intentional and documented (amortizes fsyncs), just flagging the disk-footprint tradeoff for long-running nodes.

Test quality is good: channel-based synchronization (no time.Sleep), meaningful assertions, and the failure-injection suite covers the rare error paths the codecov gaps point at.
• Branch: fix/tendermint-use-pebble-wal

Claude finished @ongyimeng's task in 2m 1s —— View job

Re-review: standalone consensus WAL (synchronize)

Re-reviewed the three commits pushed since the last review. They directly address the prior nits, and the changed packages build cleanly.

• fix: handle WAL file close errors (a188a81)
• fix(db/memory): avoid panic on temp path creation (ef5ed8f)
• test: cover WAL record / prune watermark decode (0383582)
• Check for regressions / new issues

Verdict

All three changes are correct. No blockers, no new issues.

repairWALTail close handling (wal_writer.go:181-209) — correctly converted to a named return (err error) with a deferred errors.Join on file.Close(). The earlier-return error paths (Stat/Truncate/Sync) are preserved and the close error is now joined rather than dropped. This resolves the CodeQL "writable file handle closed without error handling" finding. ✅

Path() no longer panics (db/memory/db.go:112-115) — returns "" on MkdirTemp failure instead of panic. Callers already treat "" as "no local path" (NewTendermintWALStore returns a clean error), so a transient TMPDIR failure now degrades gracefully instead of crashing the node from an infallible accessor. ✅

New decode-path tests (prune_watermark_test.go, codec_test.go, export_test.go) are meaningful — round-trip plus corruption cases (wrong size, bad header) assert real error behavior, using t.TempDir() with no flaky sleeps. ✅

Nit (non-blocking, optional)

• db/memory/db.go Path() — the prior review also suggested a doc comment noting that under an in-memory backing DB the consensus WAL lives in an ephemeral temp dir that is RemoveAll'd on Close(), so there is no crash recovery in that configuration. The panic fix is the important part and is done; a one-line contract comment on Path() would still help future readers understand the silent ephemeral-WAL behavior. Add the comment →

Overall: the PR remains in good shape — durability invariants intact, the prior review's concrete items are now closed.

Claude finished @ongyimeng's task in 2m 27s —— View job

PR Re-review: standalone consensus WAL (synchronize)

Re-reviewed the commits pushed since the last review:

• test: cover WAL record and prune watermark decode paths (0383582)
• refactor: use constant for estimatedRecordBytes (daddcff)
• refactor: pass WalRecordEnvelope by reference (1ad21e6)
• Re-check core WAL files for correctness/concurrency
• Verify the refactors introduce no aliasing/regression
• Build affected packages

These directly address @rodrodros's review nits. Affected packages (consensus/db, db/memory) build cleanly.

Verdict

All changes are correct. No blockers, no new issues.

encodeBatch index-based header write (consensus/db/codec.go:52-78) — the previous append-based header construction was replaced with slices.Grow + explicit offset writes. I verified the byte layout is byte-for-byte identical to the original:

slices.Grow(encodedBatch, recordHeaderBytes) guarantees cap ≥ headerIndex+11, so encodedBatch[:valueIndex] is safe, and all 11 reserved header bytes are unconditionally overwritten (kind/keyLen by assignment, key by PutUint32, value-length by putFixedUvarint32), so stale bytes from reused capacity can't leak. ✅

appendWALRecordPayload(payload, *walRecordEnvelope) (codec.go:83-85) — now takes the envelope by pointer to avoid copying the large struct. The function only reads record.* fields, never mutates or retains them, and &records[i] in the loop is a read-only borrow, so there's no aliasing hazard. The single test caller (export_test.go:31) was updated to pass &envelope. ✅

estimatedRecordBytes const (codec.go:40) — folding it into the const block with derived offset constants is purely mechanical and computes the same value (11 + 128 = 139). ✅

Path() returns "" on MkdirTemp failure (db/memory/db.go:114) — the fmt import was dropped along with the panic; matches the graceful-degradation behavior agreed in the prior review. ✅

The error-message wording flagged earlier (codec.go:176) is also now the clearer "trailing WAL record bytes: %d".

Overall: the PR remains in good shape. The durability invariants verified in the earlier reviews are untouched, and the refactors are behavior-preserving.
• Branch: fix/tendermint-use-pebble-wal

Claude finished @ongyimeng's task in 2m 44s —— View job

PR Re-review: standalone consensus WAL (synchronize)

Reviewed the commits pushed since the last review:

• refactor: remove impossible WAL payload size check (2f5c590)
• refactor: remove impossible WAL payload nil checks (f75177a)
• refactor: pass WAL proposal value by reference (5884660)
• fix: clarify remaining bytes error (afe4c34)
• refactor: rename consensus db package to walstore (bd2d95d)
• Verify build + check for regressions

These address @rodrodros's remaining review nits. consensus/... and db/... build cleanly, and there are no leftover references to the old consensus/db import path.

Verdict

All changes are correct and behavior-preserving. No blockers, no new issues.

Package rename db → walstore (consensus/walstore/*) — purely mechanical: package clause + every importer (consensus.go, driver/driver.go, tests) updated consistently. No stale consensus/db references remain anywhere in the tree. ✅

Removed nil guards in appendWALRecordPayload (codec.go:89-103) — safe. Every envelope reaches the encoder via setEntry (record.go:39-80), which sets EntryKind and its matching pointer (ProposalEntry/PrevoteEntry/…) together and rejects a nil typed entry up front. So a given EntryKind branch can never observe a nil payload pointer; the guards were dead code. The proposal path also still has its own Value == nil check before dereferencing. ✅

Removed valueLen > math.MaxUint32 check (codec.go:71) — a per-record value is a fixed 4×uint64 array (≤ a few hundred bytes after framing), so it can never approach MaxUint32. The genuinely-meaningful bound, len(records) > math.MaxUint32, is retained at codec.go:27, so math/errors imports are still used and the build is intact. ✅

appendValue / valueToUint64Array now take *V (codec.go:325, :429) — the sole caller passes proposalMessage.Value (already a *V), which is guaranteed non-nil by the preceding if proposalMessage.Value == nil branch, so the new reflect.ValueOf(value).Elem() can't panic on a nil pointer. The helper only reads the array via reflection — no mutation or retention — so passing by pointer avoids the 4-word copy with no aliasing hazard. ✅

Error wording (codec.go:162) — now "unexpected remaining WAL record bytes: %d", matching the prior request for a clearer message. ✅

Overall: the PR remains in good shape. The durability/replay invariants verified in earlier reviews are untouched, and this batch is a clean set of nit-resolving refactors.
• Branch: fix/tendermint-use-pebble-wal

Please hold on for abit, I’m still doing some final checks and cleaning up a few things.

Claude finished @ongyimeng's task in 2m 41s —— View job

PR Re-review: standalone consensus WAL (synchronize)

Reviewed the commits pushed since the last review:

• refactor: combine duplicate WAL number checks (b824b41)
• refactor: pass WAL uint64 arrays by reference (9d22903)
• refactor: remove redundant WAL entry checks (d472a8e)
• Verify build + check for regressions

These close out the last of @rodrodros's review nits. consensus/... builds cleanly.

Verdict

All changes are correct and behavior-preserving. No blockers, no new issues.

appendUint64Array now takes *T (codec.go:317) — avoids copying the 4-word array per call. Both call sites pass a guaranteed-non-nil address:

• appendMessageHeader passes &header.Sender — header is a value parameter, so the field is addressable. ✅
• appendVotePayload passes vote.ID only after the if vote.ID == nil { return … } guard at codec.go:301, so the pointer is never nil when dereferenced. ✅

The helper only reads (*value)[i] — no mutation or retention — so there's no aliasing hazard.

entry() drops the per-kind nil checks and returns a plain wal.Entry (panic on unknown kind) (record.go:82-97) — consistent with the other "impossible check" removals in this PR, and safe:

• On the encode side, every envelope is built by setEntry (record.go:39-77), which sets EntryKind and its matching typed pointer together.
• On the decode side (codec.go:178-214), the kind switch sets each typed pointer from decodeProposalRecord / decodeVotePayload / decodeTimeoutRecord, all of which return a non-nil pointer whenever err == nil (verified at codec.go:246, :266, :290). An unknown kind returns an error at codec.go:213 before any envelope with that kind escapes decode, so entry()'s default: panic is genuinely unreachable in practice.

So the two callers — replay.go:105 and wal_index.go:47 — can't observe a nil entry, and dropping the returned error (and the prior panic(err) in updateIndexesFromCommittedRecords) is correct. ✅

addIfMissing combined conditionals (wal_index.go:23) — s.first == walNum || slices.Contains(s.rest, walNum) is logically identical to the two separate ifs it replaced; short-circuit order is preserved. ✅

Overall: the PR remains in good shape. The durability/replay invariants verified in the earlier reviews are untouched, and this batch is a clean set of nit-resolving refactors.
• Branch: fix/tendermint-use-pebble-wal

Claude finished @ongyimeng's task in 1m 51s —— View job

PR Re-review: standalone consensus WAL (synchronize)

Reviewed the two commits pushed since the last review:

• refactor: clarify WAL batch record indexing (595f11f)
• test: assert memory DB copy path after copy (1afb6fd)
• Check for regressions / new issues
• Build affected packages

consensus/walstore and db/memory build cleanly.

Verdict

Both changes are correct and behavior-preserving. No blockers, no new issues.

encodeBatch variable rename + comment (consensus/walstore/codec.go:31-78) — purely cosmetic. keyBytes→keySizeBytes and the headerIndex/valueLenIndex/valueIndex locals were renamed to recordStart/keyLenStart/keyStart/valueLenStart/payloadStart, and a layout comment ([kind:1][keyLen:1][key:4][valueLen:5][payload...]) was added. The computed offsets are byte-for-byte identical to before:

slices.Grow(…, recordHeaderBytes) still guarantees capacity for the 11-byte header, encodedBatch[:payloadStart] is safe, and putFixedUvarint32(encodedBatch[valueLenStart:payloadStart], …) still writes exactly the reserved 5 bytes. No logic change. ✅

TestMemoryDBCopyGetsIndependentPath (db/memory/db_test.go:30-36) — implements @RafaelGranza's nit: drops the pre-captured originalPath and asserts require.NotEqual(t, memoryDB.Path(), copyPath) at the point of comparison. This is a stronger check — Copy() builds a fresh Database with an empty path (db.go:230-243), and each Path() call lazily MkdirTemps an independent dir (db.go:112-118), so the two paths are guaranteed distinct. Reading memoryDB.Path() after the copy also guards against any future change where Copy() might carry the parent's path forward. ✅

Overall: the PR remains in good shape. The durability/replay invariants verified in earlier reviews are untouched, and this batch is a clean readability + test-hardening pass.

• Branch: fix/tendermint-use-pebble-wal

Claude finished @ongyimeng's task in 2m 32s —— View job

PR Re-review: standalone consensus WAL (synchronize)

Reviewed the change pushed since the last review:

• Identify new commits since last review
• Check the main merge for conflicts/regressions in consensus/walstore
• Verify affected packages build
• Report findings

The only new commit since the last review is the main merge (892cec6e). It brings main up to date but does not touch any WAL code — the walstore package, driver integration, and db layer changes are all untouched.

Verdict

No blockers, no new issues. The WAL durability/replay invariants verified across the earlier reviews are unchanged.

Merge adaptations — within the PR's scope, the merge only removed a now-deleted CandidateTxs field from three struct literals to match main:

These are purely mechanical field removals tracking the upstream deletion of CandidateTxs, unrelated to the WAL change. ✅

Build — ./consensus/... ./sync/... ./db/... all compile cleanly after the merge. ✅

Overall: the merge is clean and the PR remains in good shape. Nothing further to flag.
• Branch: fix/tendermint-use-pebble-wal